Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the MemFlags internal representation #8162

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

alexcrichton
Copy link
Member

This commit refactors the internal MemFlags bits to be not just flags.
Instead some bits are now grouped together and interpreted as a unit.
This enables two primary API changes:

  • First the heap/table/vmctx split is now represented as an "alias
    region" which is set as an enum. This means that all MemFlags carry
    an Option<AliasRegion> internally.

  • Second trapping state is now represented as an Option<TrapCode>.
    This means that notrap is no longer a flag and tabletrap is no
    longer a flag. This does enable storing arbitrary trap codes though so
    long as they aren't TrapCode::User(_).

The main purpose of this commit is to enable using more trap codes with
MemFlags for when a segfault is detected. For example with #5291 we
want a segfault to indicate a call-to-null, which is not currently
covered by MemFlags.

This commit refactors the internal `MemFlags` bits to be not just flags.
Instead some bits are now grouped together and interpreted as a unit.
This enables two primary API changes:

* First the `heap`/`table`/`vmctx` split is now represented as an "alias
  region" which is set as an enum. This means that all `MemFlags` carry
  an `Option<AliasRegion>` internally.

* Second trapping state is now represented as an `Option<TrapCode>`.
  This means that `notrap` is no longer a flag and `tabletrap` is no
  longer a flag. This does enable storing arbitrary trap codes though so
  long as they aren't `TrapCode::User(_)`.

The main purpose of this commit is to enable using more trap codes with
`MemFlags` for when a segfault is detected. For example with bytecodealliance#5291 we
want a segfault to indicate a call-to-null, which is not currently
covered by `MemFlags`.
Instead use the `AliasRegion` enum instead to help emphasize that only
one region is possible on a `MemFlags`, not multiple.
@alexcrichton alexcrichton requested review from a team as code owners March 18, 2024 04:13
@alexcrichton alexcrichton requested review from fitzgen and removed request for a team March 18, 2024 04:13
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:wasm labels Mar 18, 2024
Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, after one minor doc-comment fix I think. Very clear and reviewable, thank you!

cranelift/codegen/src/ir/memflags.rs Outdated Show resolved Hide resolved
Co-authored-by: Jamey Sharp <jamey@minilop.net>
@alexcrichton alexcrichton added this pull request to the merge queue Mar 18, 2024
Merged via the queue into bytecodealliance:main with commit b651c44 Mar 18, 2024
19 checks passed
@alexcrichton alexcrichton deleted the refactor-mem-flags branch March 18, 2024 15:39
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Mar 18, 2024
This commit uses the support from bytecodealliance#8162 to skip null function pointer
checks when performing an indirect call. Instead of an explicit check
the segfault from accessing the null function pointer is caught and
annotated with the appropriate trap.

Closes bytecodealliance#5291
github-merge-queue bot pushed a commit that referenced this pull request Mar 18, 2024
This commit uses the support from #8162 to skip null function pointer
checks when performing an indirect call. Instead of an explicit check
the segfault from accessing the null function pointer is caught and
annotated with the appropriate trap.

Closes #5291
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:wasm cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants